Reviewed 1 of 17 files at r2, 1 of 1 files at r3.
Reviewable status: 2 of 66 files reviewed, 5 unresolved discussions (waiting on @pronebird)

ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift line 138 at r2 (raw file):

                parsedOptions.selectorResult = selectorResult
            } else {
                parsedOptions.launchSource = tunnelOptions.isOnDemand() ? .onDemand : .system

This should already be the default (from init), so no need to set it again.

ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift line 112 at r4 (raw file):

    }

    override func wake() {

Why not keeping to async here?

ios/PacketTunnelCore/Actor/Actor.swift line 178 at r3 (raw file):

     - Parameter date: date when last key rotation took place.
     */
    public func notifyKeyRotated(date: Date? = nil) async {

Nit: This is perhaps more of a "notifyKeyRotation" rather than the key actually have been rotated. If rotation fails (or is attempted), the actor will still be notified with a "last rotation date".

ios/PacketTunnelCore/Actor/Actor.swift line 196 at r3 (raw file):

            case var .connecting(connState):
                if mutateConnectionState(&connState) {
                    state = .connecting(connState)

Why do we set the same state again?

ios/PacketTunnelCore/Actor/Actor.swift line 231 at r3 (raw file):

    /**
     Cients should call this method to notify actor when device becomes awake.

Nit: "Clients"

Reviewed 20 of 63 files at r1, 2 of 17 files at r2, all commit messages.
Reviewable status: 24 of 66 files reviewed, 8 unresolved discussions (waiting on @pronebird)

ios/PacketTunnel/PacketTunnelProvider/SettingsReader.swift line 49 at r4 (raw file):

private extension DNSSettings {
    /**
     Converys `DNSSettings` to `SelectedDNSServers` structure.

Nit: "Converts"

ios/PacketTunnelCore/Actor/State.swift line 81 at r4 (raw file):

    /// Error state.
    /// This state is normally entered when the tunnel is unable to start or reconnect.
    /// In this state the tunnel blocks all nework connectivity by setting up a peerless WireGuard tunnel, and either awaits user action or, in cirtain

Nit: "Certain"

ios/PacketTunnelCore/Actor/TaskQueue.swift line 74 at r4 (raw file):

    /// Remove task by ID previously returned by call to `registerTask()`.
    private func unregisterTask(_ taskId: TaskId) {
        let index = queuedTasks.firstIndex { $0.id == taskId }

Can be simplified: queuedTasks.removeAll { $0.id == taskId }

Reviewable status: 22 of 68 files reviewed, 8 unresolved discussions (waiting on @rablador)

ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift line 138 at r2 (raw file):

Different value is being set. This branch infers that if relay selector is not present then it means the system launched the packet tunnel. I refined the else branch. Please double check.

Reviewable status: 21 of 68 files reviewed, 6 unresolved discussions (waiting on @rablador)

ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift line 112 at r4 (raw file):

wake() is defined as non-async in the superclass.

ios/PacketTunnel/PacketTunnelProvider/SettingsReader.swift line 49 at r4 (raw file):

Done.

ios/PacketTunnelCore/Actor/Actor.swift line 196 at r3 (raw file):

The actor only mutates the associated value (ConnectionState) in here. It will later call reconnectTunnel() (see the task created in startKeySwitchTask())

ios/PacketTunnelCore/Actor/Actor.swift line 231 at r3 (raw file):

Was it fixed?

ios/PacketTunnelCore/Actor/State.swift line 81 at r4 (raw file):

Done

ios/PacketTunnelCore/Actor/TaskQueue.swift line 74 at r4 (raw file):

removeAll() iterates over all of elements in array that why I prefer firstIndex() + remove(at:) combo since it's expected to have only one task with the same ID in the queue.

Reviewable status: 20 of 68 files reviewed, 10 unresolved discussions (waiting on @pronebird and @rablador)

ios/PacketTunnelCore/Actor/Actor.swift line 398 at r4 (raw file):

     */
    private func tryStart(nextRelay: NextRelay = .random) async throws {
        func makeConnectionState(settings: Settings) throws -> ConnectionState? {

This is a big enough function to warrant standing on its own, was there a specific reason to inline it like this ?

ios/PacketTunnelCore/Actor/Actor.swift line 572 at r4 (raw file):

     - Parameter reason: reason why the actor is entering error state.
     */
    private func setErrorStateInner(with reason: BlockedStateReason) async {

nit
I don't know if it's worth, but we can use the swift 5.9 "switch as expression" feature to avoid repeating the state change like so

private func setErrorStateInner(with reason: BlockedStateReason) async {
    let blockedState: BlockedState? = switch state {
    case .initial:
        BlockedState(
            reason: reason,
            relayConstraints: nil,
            currentKey: nil,
            keyPolicy: .useCurrent,
            networkReachability: defaultPathObserver.defaultPath?.networkReachability ?? .undetermined,
            recoveryTask: startRecoveryTaskIfNeeded(reason: reason),
            priorState: state.priorState!
        )

    case let .connected(connState), let .connecting(connState), let .reconnecting(connState):
        BlockedState(
            reason: reason,
            relayConstraints: connState.relayConstraints,
            currentKey: nil,
            keyPolicy: connState.keyPolicy,
            networkReachability: connState.networkReachability,
            priorState: state.priorState!
        )

    // Funky / ugly syntax here, but necessary to return a single value that the compiler can type check
    case var .error(blockedState): { 
            if blockedState.reason != reason {
                blockedState.reason = reason
            }
            return blockedState
        }()

    case .disconnecting, .disconnected:
        nil
    }

    if let blockedState {
        state = .error(blockedState)
        await configureAdapterForErrorState()
    }
}

ios/PacketTunnelCore/Actor/Actor.swift line 379 at r5 (raw file):

// MARK: - Network Reachability

It would be nice to split this into multiple files if possible
like Actor+Reachability etc...

ios/PacketTunnelCore/Actor/Actor.swift line 637 at r5 (raw file):

        case .random:
            return try relaySelector.selectRelay(
                with: relayConstraints,

Shouldn't this be any relay constraints in case of the .random selection ? Or are we trying to still respect whatever constraint was chosen by the user here ?

Reviewable status: 20 of 68 files reviewed, 10 unresolved discussions (waiting on @buggmagnet and @rablador)

ios/PacketTunnelCore/Actor/Actor.swift line 398 at r4 (raw file):

No particular reason I think. I'll move it into separate function.

ios/PacketTunnelCore/Actor/Actor.swift line 572 at r4 (raw file):

Good idea let me try it.

ios/PacketTunnelCore/Actor/Actor.swift line 379 at r5 (raw file):

Ok I wasn't sure but I'll look into doing that.

ios/PacketTunnelCore/Actor/Actor.swift line 637 at r5 (raw file):

The latter. "random" means to pick a random relay satisfying constraints chosen by user. If constraints yield zero relays then we enter blocked state (error).

Reviewed 8 of 63 files at r1, 5 of 17 files at r2, 1 of 14 files at r5.
Reviewable status: 31 of 73 files reviewed, 10 unresolved discussions (waiting on @buggmagnet and @pronebird)

ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift line 138 at r2 (raw file):

Ah, I'm silly, didn't actually read properly...

ios/PacketTunnel/WireGuardAdapter/WgAdapter.swift line 54 at r4 (raw file):

        let dispatchGroup = DispatchGroup()
        dispatchGroup.enter()

Why do we use a group for this?

ios/PacketTunnelCore/Actor/Actor.swift line 196 at r3 (raw file):

Yeah, I saw this during test session with @buggmagnet today. Nevermind, lost myself in code and just missed that param was inout.

ios/PacketTunnelCore/Actor/State+Extensions.swift line 119 at r4 (raw file):

    var shouldRestartAutomatically: Bool {
        switch self {
        case .deviceLocked, .outdatedSchema:

@buggmagnet here, typing on my keyboard.
In practice, whenever we return.outdatedSchema, we need the user to restart the app for the schema update to apply.
This means we will get stuck in a boot loop, endlessly trying to start (and fail) the tunnel. I'm not sure this is the correct approach in this case.
We should probably return false here, and wait for the user to relaunch the app, since we will already be in a blocked state.

Reviewed 1 of 1 files at r6, 1 of 2 files at r7, 1 of 7 files at r8, all commit messages.
Reviewable status: 34 of 73 files reviewed, 8 unresolved discussions (waiting on @buggmagnet and @pronebird)

Reviewable status: 33 of 73 files reviewed, 8 unresolved discussions (waiting on @buggmagnet and @rablador)

ios/PacketTunnel/WireGuardAdapter/WgAdapter.swift line 54 at r4 (raw file):

It's used to block current thread until either the response is received from adapter.getRuntimeConfiguration { .. } or timeout (1 second). Tunnel Monitor calls this function to obtain WG adapter stats (bytes received/sent) etc..

Reviewable status: 32 of 73 files reviewed, 8 unresolved discussions (waiting on @buggmagnet and @pronebird)

ios/PacketTunnel/WireGuardAdapter/WgAdapter.swift line 54 at r4 (raw file):

Initially felt strange to use a group for a single value, but I guess it's a convenient to observe both the result and timeout simultaneosly.

Reviewed 5 of 19 files at r11.
Reviewable status: 63 of 78 files reviewed, 12 unresolved discussions (waiting on @buggmagnet and @pronebird)

ios/PacketTunnelCore/Actor/CommandChannel.swift line 28 at r11 (raw file):

 Consuming commands can be implemented using a for-await loop. Note that using a loop should also serialize the command handling as the next command will not
 be consumed until the body of the loop does not complete the iteration.

"does not complete" -> "completes"

ios/PacketTunnelCore/Actor/CommandChannel.swift line 153 at r11 (raw file):

            defer { i -= 1 }

            assert(i < buffer.count)

Can this ever be false?

Reviewed 2 of 19 files at r11, 4 of 8 files at r12, all commit messages.
Reviewable status: 69 of 78 files reviewed, 10 unresolved discussions (waiting on @pronebird and @rablador)

ios/PacketTunnel/WireGuardAdapter/WireGuardAdapter+Async.swift line 14 at r11 (raw file):

Unless we measure it, saying "this has overhead" is usually not a good argument, or a good enough reason to use an unsafe API over a safe one.
Please use the checked version instead.

ios/PacketTunnelCore/Actor/Actor.swift line 92 at r11 (raw file):

Considering this is a nonisolated function that schedules a detached task that we don't keep around, we will not be able to cancel it currently. This means that the inner Task.sleep also won't get cancelled either.
In that case, please make private func reconnect(to nextRelay: NextRelay, shouldStopTunnelMonitor: Bool) non throwing

ios/PacketTunnelCore/Actor/Actor.swift line 272 at r11 (raw file):

Eh let's scratch that for now, I hadn't realized that RelaySelectorWrapper was defined in PacketTunnel.
IMHO it shouldn't, and should stay in the RelaySelector package.

ios/PacketTunnelCore/Actor/Actor+KeyPolicy.swift line 60 at r12 (raw file):

                if let currentKey = blockedState.currentKey {
                    blockedState.lastKeyRotation = lastKeyRotation
                    blockedState.keyPolicy = .usePrior(currentKey, startKeySwitchTask())

I'm not sure I understand the logic here, why do we force a key switch when we are in the blocked state ?
Can you write a comment that explains why ?

ios/PacketTunnelCore/Actor/Actor+KeyPolicy.swift line 154 at r12 (raw file):

            return false

        case let .usePrior(_, autoCancellingTask):

⚠️ Immutable value 'autoCancellingTask' was never used; consider replacing with '_' or removing it

Reviewed 13 of 63 files at r1, 11 of 17 files at r2, 10 of 14 files at r5, 9 of 19 files at r11, 4 of 8 files at r12, 3 of 3 files at r13, all commit messages.
Reviewable status: all files reviewed, 25 unresolved discussions (waiting on @pronebird)

ios/PacketTunnelCore/Actor/Actor+SleepCycle.swift line 17 at r12 (raw file):

     `NEPacketTunnelProvider` provides the corresponding lifecycle method.

     - Important: It's safe to call this method from any thread.

nit

I think we don't need to point out when a method is thread safe.
This should already be implied by the nonisolated function (since it means it cannot modify the isolated parts in the actor).
If you look at Apple documentation, they usually say something along the lines of "All the methods in XXX are thread safe"

Essentially, we should assume thread safety by default, and specify when something is not thread safe, since documentation is hard to read,
it's better to only point out the important stuff in doc to avoid cognitive load when reading documentation.

ios/PacketTunnelCore/Actor/Command.swift line 20 at r12 (raw file):

    /// Reconnect tunnel.
    /// `stopTunnelMonitor = false` is only used when tunnel monitor is paused in response to connectivity loss and shouldn't be stopped explciitly,

typo
explciitly

ios/PacketTunnelCore/Actor/Command.swift line 40 at r12 (raw file):

    /// Format command for log output.
    func logFormat() -> String {

Why not make Command conform to CustomDebugStringConvertible instead ?

ios/PacketTunnelCore/Actor/ConfigurationBuilder.swift line 37 at r12 (raw file):

        return TunnelPeer(
            endpoint: .ipv4(endpoint.ipv4Relay),
            publicKey: PublicKey(rawValue: endpoint.publicKey)!

We already make this var return TunnelPeer?
So let's not force unwrap here, but return nil if we couldn't construct PublicKey

ios/PacketTunnelCore/Actor/State+Extensions.swift line 24 at r12 (raw file):

            return .reconnecting
        case .disconnecting, .disconnected, .error:
            return nil

Since this pretty much should never happen, and if it did, it would be a serious programming mistake, how about we put a fatalError (with a pretty clear error message) here instead, and make the return type non optional ?

ios/PacketTunnelCore/Actor/State+Extensions.swift line 56 at r12 (raw file):

    // MARK: - Logging

    func logFormat() -> String {

Same remark about CustomDebugStringConvertible

ios/PacketTunnelCore/Actor/Protocols/RelaySelectorProtocol.swift line 14 at r12 (raw file):

/// Protocol describing a type that can select a relay.
public protocol RelaySelectorProtocol {

This file should really be in RelaySelector and not in PacketTunnelCore

ios/PacketTunnelCore/TunnelMonitor/TunnelMonitorProtocol.swift line 22 at r12 (raw file):
nit
When defining a protocol, we don't need to say "protocol that..."
We're already in the context, we know we're dealing with a protocol

We can simply say "A type that can provide tunnel monitoring" here for instance.

There are 5 instances of the phrase "protocol that" at the time of writing this, so not a big deal, but let's not make a habit out of this either.

For example, the Error protocol starts as such

A type representing an error value that can be thrown.

ios/PacketTunnelCoreTests/ActorTests.swift line 112 at r12 (raw file):

     4. An actor should transition through `.connecting` towards`.connected` state.
     */
    func testLockedDeviceErrorOnBoot() async throws {

This test is currently broken since the introduction of channels. I haven't investigated why, but let's make sure to fix this.

ios/PacketTunnelCoreTests/TaskSleepTests.swift line 20 at r12 (raw file):

        task.cancel()

        do {

nit
I would have suggested the following simplification

XCTAssertThrowsError(Task { try await task.value }) { error in
    XCTAssertTrue(error is CancellationError)
}

BUT that makes the test fail, because suddenly the operation becomes non throwing.
Go figure ¯\_ (ツ)_/¯

ios/PacketTunnelCoreTests/Mocks/MockRelaySelector.swift line 28 at r12 (raw file):

}

extension MockRelaySelector {

We should consolidate all the mocked relays we defined in RelaySelectorTests in here (if possible)
namely the (very long) private let sampleRelays = REST.ServerRelaysResponse( variable

ios/PacketTunnelCoreTests/Mocks/MockSettingsReader.swift line 15 at r12 (raw file):

import class WireGuardKitTypes.PrivateKey

struct MockSettingsReader: SettingsReaderProtocol {

nit
This is kind of a big ask, so we don't need to do this here BUT Mocks are not stubs.

Now I'm being super pedantic here, but ideally those should named Stubs probably.

Since we already have expectations built in XCTest we don't really need mocks in general.

ios/PacketTunnelCoreTests/Mocks/MockTunnelMonitor.swift line 80 at r12 (raw file):

            self?.dispatch(event, after: delay)
        }
        simulationBlock(.start, dispatcher)

This should be simulationBlock(command, dispatcher)

ios/PacketTunnelCoreTests/Mocks/MockTunnelMonitor.swift line 89 at r12 (raw file):

        MockTunnelMonitor { command, dispatcher in
            if case .start = command {
                dispatcher.send(.connectionEstablished, after: .milliseconds(100))

We can easily go down to 50ms or even 10ms here, gotta go fast ! 🚀

ios/PacketTunnelCoreTests/Mocks/PacketTunnelActor+Mocks.swift line 15 at r12 (raw file):

    static var timingsForTests: PacketTunnelActorTimings {
        return PacketTunnelActorTimings(
            bootRecoveryPeriodicity: .milliseconds(100),

Can we lower those to smaller values like 50ms or 10ms ?

ios/PacketTunnelCore/Actor/StartOptions.swift line 27 at r12 (raw file):

    /// Returns a brief description suitable for output to tunnel provider log.
    public func logFormat() -> String {

Same remark as in another file, let's use CustomDebugStringConvertible instead

Reviewable status: 65 of 78 files reviewed, 25 unresolved discussions (waiting on @buggmagnet and @rablador)

ios/PacketTunnel/WireGuardAdapter/WireGuardAdapter+Async.swift line 14 at r11 (raw file):

Done.

ios/PacketTunnelCore/Actor/Actor.swift line 178 at r3 (raw file):

Fixed

ios/PacketTunnelCore/Actor/Actor.swift line 92 at r11 (raw file):

Fixed.

Yeah, there is a bit of a chicken and egg problem with that task as it calls into "self". Maybe adding explicit call to start handling commands would be better?

ios/PacketTunnelCore/Actor/Actor+KeyPolicy.swift line 60 at r12 (raw file):

Because key rotation may still happen while in blocked state and because key policy is preserved between states.

However:

• The tunnel should not be reconnected automatically when in blocked state at the time of transition from .usePrior to .useCurrent.
• We should consume currentKey when moving it into .usePrior key policy to prevent it from being reused later.

I have addressed that in a separate commit.

ios/PacketTunnelCore/Actor/Actor+KeyPolicy.swift line 154 at r12 (raw file):

Done.

ios/PacketTunnelCore/Actor/Actor+SleepCycle.swift line 17 at r12 (raw file):

Deep. Fixed.

ios/PacketTunnelCore/Actor/Command.swift line 40 at r12 (raw file):

Because we don't use debugPrint(). logFormat() is used specifically for log output that we receive with problem reports.

ios/PacketTunnelCore/Actor/CommandChannel.swift line 28 at r11 (raw file):

Done.

ios/PacketTunnelCore/Actor/CommandChannel.swift line 153 at r11 (raw file):

Only if coalescing algo within the loop does not properly adjust i.

ios/PacketTunnelCore/Actor/ConfigurationBuilder.swift line 37 at r12 (raw file):

@rablador raised and fixed this today, perhaps he could push his changes on top.

In short if we cannot construct PublicKey() from raw data, then we should enter blocked state. Returning nil here would configure empty WG tunnel which is not the intention when endpoint is provided.

This is extreme edge case as that means that our relay list contains malformed public keys. In conversation with @rablador we figured that throwing an error from here would be better as it would bubble up to tryStart() and cause actor to enter error state.

ios/PacketTunnelCore/Actor/State+Extensions.swift line 24 at r12 (raw file):

True but I am not very fond of fatal errors esp. in the tunnel. Since this is only used in a single function within Actor+ErrorState, I figured that we could solve it with a helper function and pass the priorState manually from within switch case.

ios/PacketTunnelCore/Actor/Protocols/RelaySelectorProtocol.swift line 14 at r12 (raw file):

It's a direct dependency of PacketTunnelActor and implemented in PacketTunnel by RelaySelectorWrapper. RelaySelector could live without this protocol, actor cannot. So I think we should keep it where it's being used/consumed.

ios/PacketTunnelCoreTests/ActorTests.swift line 112 at r12 (raw file):

Some of the changes I have introduced lately mutate state in a few steps vs. one which causes more updates than anticipated before. Connecting state fires twice and the corresponding expectation fails because it only expects to be fulfilled once. So I think I will have to look how I register state transitions using expectations.

ios/PacketTunnelCoreTests/TaskSleepTests.swift line 20 at r12 (raw file):

XCTAssert family of functions accept expressions with @autoclosure which is defined as non-async function so not much we can do about that unless we roll our own using continuations and wrap everything in custom helpers I guess.

ios/PacketTunnelCoreTests/Mocks/MockRelaySelector.swift line 28 at r12 (raw file):

Maybe we could put them in JSON instead.

ios/PacketTunnelCoreTests/Mocks/MockTunnelMonitor.swift line 80 at r12 (raw file):

Done.

Reviewed 4 of 13 files at r14, all commit messages.
Reviewable status: 69 of 78 files reviewed, 19 unresolved discussions (waiting on @pronebird and @rablador)

ios/PacketTunnelCore/Actor/Actor+KeyPolicy.swift line 60 at r12 (raw file):

But if we're in a blocked state, we will be blocking all traffic, as done so in configureAdapterForErrorState

// Configure tunnel with empty WireGuard configuration that consumes all traffic on device
// emulating a firewall blocking all traffic.

private func configureAdapterForErrorState() async {
    do {
        let configurationBuilder = ConfigurationBuilder(
            privateKey: PrivateKey(),
            interfaceAddresses: []
        )
        try await tunnelAdapter.start(configuration: configurationBuilder.makeConfiguration())
    } catch {
        logger.error(error: error, message: "Unable to configure the tunnel for error state.")
    }
}

So key rotations shouldn't work in the first place. Or am I misunderstanding something ?
Or does this mean key rotation operations will leak outside of the tunnel ?

Reviewable status: 69 of 78 files reviewed, 15 unresolved discussions (waiting on @pronebird and @rablador)

ios/PacketTunnelCore/Actor/Command.swift line 20 at r12 (raw file):

typo
explcitly 🙃

ios/PacketTunnelCore/Actor/Command.swift line 40 at r12 (raw file):

That's doing exactly the same thing with CustomStringConvertible though

struct Point: CustomStringConvertible {
    var description: String { "x:\(x) y:\(y)" }
    
    let x: Int
    let y: Int

    func logFormat() -> String {
        "x:\(x) y:\(y)"
    }
}

let p = Point(x: 1, y: 2)
print(p) // x:1 y:2
print(p.logFormat()) // x:1 y:2

Not a big deal I guess, but not really necessary to complicate our lives that way either.

ios/PacketTunnelCore/Actor/Protocols/RelaySelectorProtocol.swift line 14 at r12 (raw file):

The problem with that approach is that this creates an implicit chain dependency between PacketTunnelCore andRelaySelector.

Let's say package XYZ is consuming PacketTunnelCore which declares this protocol, now it also has to consume RelaySelector even if it doesn't need it.

In our case, it's not a big deal since RelaySelector is pretty much everywhere, but otherwise, I would say that this breaks the Interface Segregation Principle (No code should depend on interfaces it does not use)

Not going to be blocking on that, but I don't encourage this either.

Reviewable status: 69 of 78 files reviewed, 15 unresolved discussions (waiting on @pronebird and @rablador)

ios/PacketTunnelCore/Actor/Actor+KeyPolicy.swift line 60 at r12 (raw file):

Nevermind, I had forgotten that the packet tunnel process itself will still have internet, and so technically, we can still try key rotation operations in a blocked state.

Reviewable status: 69 of 78 files reviewed, 15 unresolved discussions (waiting on @buggmagnet and @rablador)

ios/PacketTunnelCore/Actor/Actor+KeyPolicy.swift line 60 at r12 (raw file):

That's correct. We may still rotate the key bypassing VPN.

ios/PacketTunnelCore/Actor/Protocols/RelaySelectorProtocol.swift line 14 at r12 (raw file):

Let's say package XYZ is consuming PacketTunnelCore which declares this protocol, now it also has to consume RelaySelector even if it doesn't need it.

It has to be consumed either way because RelaySelectorResult is defined in RelaySelector.

In our case, it's not a big deal since RelaySelector is pretty much everywhere, but otherwise, I would say that this breaks the c (No code should depend on interfaces it does not use)

Technically this can fixed. I can have a look at how the output requirement can be formalized.

Reviewable status: 69 of 78 files reviewed, 15 unresolved discussions (waiting on @buggmagnet and @rablador)

ios/PacketTunnelCore/Actor/Command.swift line 40 at r12 (raw file):

I did it exactly this way a few years back, and then decided in favor of explicit function to format a struct specifically for logging because it's explicit in intent and it doesn't get in a way when debugging.

For instance Date.logFormatDate() specifically formats date suitable for logging and not for output into Xcode console when debugging or in any other context.

Reviewable status: 62 of 79 files reviewed, 13 unresolved discussions (waiting on @buggmagnet and @rablador)

ios/PacketTunnelCore/Actor/Command.swift line 20 at r12 (raw file):

Done.

ios/PacketTunnelCore/TunnelMonitor/TunnelMonitorProtocol.swift line 22 at r12 (raw file):

Done.

ios/PacketTunnelCoreTests/ActorTests.swift line 112 at r12 (raw file):

This test will be re-introduced in the follow up work by you and Jon, as agreed today during the call.

Reviewable status: 54 of 83 files reviewed, 13 unresolved discussions (waiting on @buggmagnet and @rablador)

ios/PacketTunnelCore/Actor/Protocols/RelaySelectorProtocol.swift line 14 at r12 (raw file):

I have prepared a quick draft commit in 3742490 that replaces the use of RelaySelectorResult with SelectedRelay type exported from PacketTunnelCore in order to remove dependency on RelaySelector. Also it makes it possible to reconnect the tunnel using different options, i.e by passing NextRelay enum type instead of RelaySelectorResult as it used to be. I believe this can be useful.

ios/PacketTunnelCoreTests/Mocks/PacketTunnelActor+Mocks.swift line 15 at r12 (raw file):

Done.

ios/PacketTunnelCoreTests/Mocks/MockSettingsReader.swift line 15 at r12 (raw file):

As I understood it applies to all test objects. Renamed the types based on proposed nomenclature. Also put Mock/Stub/Fake at end of type name and not in front as it used to be.

ios/PacketTunnelCoreTests/Mocks/MockTunnelMonitor.swift line 89 at r12 (raw file):

Done.

Reviewed 5 of 13 files at r14, 7 of 8 files at r15, 2 of 2 files at r16, 4 of 13 files at r17, 9 of 9 files at r18, all commit messages.
Reviewable status: 81 of 83 files reviewed, 7 unresolved discussions (waiting on @pronebird and @rablador)

ios/PacketTunnelCore/Actor/Protocols/RelaySelectorProtocol.swift line 14 at r12 (raw file):

Thanks, it's very appreciated ! If we can merge it after merging this PR, that will be nice.
I will look at it soon

ios/PacketTunnelCoreTests/Mocks/MockRelaySelector.swift line 28 at r12 (raw file):

I moved it around to get it globally for tests (not super proud) since I needed it somewhere else for the TunnelManager tests, but probably in the future we could (should ?) do that. I'll think about it.

ios/PacketTunnelCoreTests/Mocks/MockSettingsReader.swift line 15 at r12 (raw file):

Really nice, thanks !

Reviewable status: 81 of 83 files reviewed, 5 unresolved discussions (waiting on @buggmagnet and @rablador)

ios/PacketTunnelCore/Actor/Protocols/RelaySelectorProtocol.swift line 14 at r12 (raw file):

Agree. Let's do it after this PR.

ios/PacketTunnelCoreTests/Mocks/MockRelaySelector.swift line 28 at r12 (raw file):

Cool. Then let's not touch anything in this PR and address it later.

Reviewable status: 73 of 87 files reviewed, 5 unresolved discussions (waiting on @buggmagnet and @rablador)

ios/PacketTunnelCore/Actor/State+Extensions.swift line 56 at r12 (raw file):

I left the answer on one of similar remarks. So I assume this is just a left over.

Reviewed 4 of 19 files at r11, 1 of 8 files at r12, 2 of 3 files at r13, 9 of 14 files at r14, 8 of 8 files at r15, 2 of 2 files at r16, 4 of 17 files at r17, 7 of 9 files at r18, 2 of 2 files at r19, 2 of 2 files at r20, 1 of 1 files at r21, 8 of 8 files at r22, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @buggmagnet and @pronebird)

ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift line 136 at r22 (raw file):

            if !allIPAddresses.isEmpty, !isLoggedSameIP {
                isLoggedSameIP = true
                logIfDeviceHasSameIP(than: allIPAddresses)

than -> as

ios/PacketTunnelCore/Actor/Actor+ConnectionMonitoring.swift line 72 at r19 (raw file):

        // Tunnel monitor should already be paused at this point so don't stop it to avoid a reset of its internal
        // counters.
        commandChannel.send(.reconnect(.random, stopTunnelMonitor: false))

There was some talk about adding something like a ReconnectionReason to reconnect command to remove the need for separately mutating state when incerementing the attempt count. We didn't really "finish" the discussion, but I guess the proposed change was put on hold then?

ios/PacketTunnelCore/Actor/Actor+KeyPolicy.swift line 95 at r19 (raw file):

    func switchToCurrentKey() {
        if switchToCurrentKeyInner() {
            commandChannel.send(.reconnect(.random))

Why not reconnect to .current? There are more instances in the code base where .random is selected.

ios/PacketTunnelCore/Actor/ConfigurationBuilder.swift line 37 at r12 (raw file):

This has been adressed a separate PR that branches off another branch (and ultimately this one). I think it's fine if we merge it after this PR has been merged.

ios/PacketTunnelCoreTests/Mocks/PacketTunnelActor+Mocks.swift line 25 at r19 (raw file):

        tunnelAdapter: TunnelAdapterProtocol = TunnelAdapterDummy(),
        tunnelMonitor: TunnelMonitorProtocol = TunnelMonitorStub.nonFallible(),
        defaultPathObserver: DefaultPathObserverProtocol = DefaultPathObserverFake(),

"Dummy"...?

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @buggmagnet and @rablador)

ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift line 136 at r22 (raw file):

This was introduced in cae5cdf#diff-ec10e6e167d767e71eba632daeb5dc0012e34846fa7d0d68051cf310f2533afdR242-R251 and I simply reintegrated the changes.

ios/PacketTunnelCore/Actor/Actor+ConnectionMonitoring.swift line 72 at r19 (raw file):

I don't think it was discussed outside of our private conversation. This can be addressed separately if you feel so. I think that I poked around a bit and the change wasn't as straightforward as I thought but I could revisit that of course.

ios/PacketTunnelCore/Actor/Actor+KeyPolicy.swift line 95 at r19 (raw file):

I believe I was told to use random to prevent any kind of potential for tracing a connection between the same client using old and new keys.

ios/PacketTunnelCoreTests/Mocks/PacketTunnelActor+Mocks.swift line 25 at r19 (raw file):

Path observer uses a memory storage to simulate path observer, so t's not a dummy but fake. @buggmagnet shared a link to the article which breaks down the differences. Did I get it wrong?

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @buggmagnet and @pronebird)

ios/PacketTunnelCore/Actor/Actor+ConnectionMonitoring.swift line 72 at r19 (raw file):

No, it was just between us. And no, let's leave it for now if it means extra work.

ios/PacketTunnelCore/Actor/Actor+KeyPolicy.swift line 95 at r19 (raw file):

Aha, didn't know that. Ok, that's fine then since we'll still be adhering to the relay constraints.

ios/PacketTunnelCoreTests/Mocks/PacketTunnelActor+Mocks.swift line 25 at r19 (raw file):

Probably not, just that it stood out as something we really don't use in the app elsewhere. Nevermind :)

Reviewed 1 of 8 files at r15, 2 of 2 files at r19, 2 of 2 files at r20, 1 of 1 files at r21, 8 of 8 files at r22, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @pronebird and @rablador)

ios/PacketTunnelCoreTests/Mocks/PacketTunnelActor+Mocks.swift line 25 at r19 (raw file):

@rablador here's the link I posted : mocks are not stubs
In most of our test cases, we use stubs (an object that returns pre baked responses)

I'm being nitpicky here and ultimately I think we'll be fine with either dummies or stubs in most of our tests.
It's just that I want to make sure we use the correct terminology so that new comers, or people who read our codebase don't get the wrong idea.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @buggmagnet and @pronebird)

ios/PacketTunnelCoreTests/Mocks/PacketTunnelActor+Mocks.swift line 25 at r19 (raw file):

Got it. According to the article, "Fake" seems correct here.